Add configurable rebalance strategy for bridge pegout#934
Open
volodymyrzahanych wants to merge 4 commits intov2.6.0from
Open
Add configurable rebalance strategy for bridge pegout#934volodymyrzahanych wants to merge 4 commits intov2.6.0from
volodymyrzahanych wants to merge 4 commits intov2.6.0from
Conversation
Dependency Review✅ No vulnerabilities or license issues or OpenSSF Scorecard issues found.Snapshot WarningsEnsure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice. OpenSSF Scorecard
Scanned Files |
Introduce UTXO_SPLIT strategy that splits bridge conversions into
multiple transactions based on BridgeTransactionMin, alongside the
existing ALL_AT_ONCE behavior. Strategy is configured via
REBALANCE_STRATEGY env variable.
66b4254 to
92b7cbf
Compare
There was a problem hiding this comment.
Pull request overview
Adds a configurable rebalance strategy to the bridge pegout flow so operators can choose between the existing single-transaction rebalance and a split-into-many-transactions approach to improve BTC-side UTXO distribution.
Changes:
- Introduces
RebalanceStrategy(ALL_AT_ONCE,UTXO_SPLIT) and dispatches behavior inBridgePegoutUseCase. - Wires the strategy from environment → registry → use case, updating constructors and tests accordingly.
- Adds unit tests covering parsing and UTXO split math/integrity.
Reviewed changes
Copilot reviewed 11 out of 11 changed files in this pull request and generated 5 comments.
Show a summary per file
| File | Description |
|---|---|
internal/usecases/pegout/bridge_pegout.go |
Implements strategy parsing + ALL_AT_ONCE vs UTXO_SPLIT execution paths and shared balance check. |
internal/usecases/pegout/bridge_pegout_test.go |
Updates existing tests for new constructor signature; adds parsing and split strategy test coverage. |
internal/configuration/environment/environment.go |
Adds REBALANCE_STRATEGY env field with validation. |
internal/configuration/environment/environment_reader_test.go |
Ensures test env setup includes REBALANCE_STRATEGY. |
internal/configuration/registry/usecase.go |
Parses strategy at startup and injects it into the bridge pegout use case; constructor now returns (*UseCaseRegistry, error). |
internal/configuration/registry/usecase_test.go |
Updates to handle NewUseCaseRegistry returning an error and sets strategy in env. |
internal/configuration/registry/watcher_test.go |
Updates to handle NewUseCaseRegistry returning an error and sets strategy in env. |
internal/adapters/entrypoints/watcher/pegout_bridge_watcher_test.go |
Updates use case construction to pass default strategy. |
cmd/application/lps/application.go |
Handles NewUseCaseRegistry returning an error at application startup. |
docker-compose/local/docker-compose.lps.yml |
Exposes REBALANCE_STRATEGY to the container environment. |
sample-config.env |
Adds REBALANCE_STRATEGY=ALL_AT_ONCE example/default. |
You can also share your feedback on Copilot code review. Take the survey.
Luisfc68
requested changes
Mar 18, 2026
Collaborator
|
@volodymyrzahanych one comment I forgot: there are multiple compose files for the LPS, pls add the env var to all of them, right now is just in the local. Thanks! |
38862d2 to
d0088bc
Compare
d0088bc to
dcb1ba4
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Add configurable rebalance strategy for bridge pegout, supporting two modes: ALL_AT_ONCE (existing behavior - converts entire RSK balance to BTC in a single transaction) and UTXO_SPLIT (splits the pegout amount into multiple UTXOs based on the provider's min/max pegout limits for better liquidity distribution)
Why
The current rebalance behavior sends all funds waiting for rebalance in a single native pegout transaction, causing UTXOs to converge into one during usage spikes
When all liquidity is consumed, only one large native pegout is executed, reducing UTXO availability for subsequent operations
Type of Change
Affected part of the project
Related Issues
Jira ticket
How to test
Set REBALANCE_STRATEGY=UTXO_SPLIT, initiate a large pegout (e.g. 3 RBTC), wait for rebalance, and verify the LP BTC wallet receives multiple UTXOs of ~BridgeTransactionMin size instead of one large UTXO
Set REBALANCE_STRATEGY=ALL_AT_ONCE, repeat the same flow, and verify the LP BTC wallet receives a single UTXO with the full amount
Run unit tests: go test ./internal/usecases/pegout/ -run TestBridgePegout
Reviewer Guidelines
firstChunk + (N-1)×bridgeMin = totalValueupdateQuotesstoring only last tx hash needs a follow-up ticketBridgeTransactionMinin management API matches the Bridge's actual minimumDiv()should use explicit_or add a commentREBALANCE_STRATEGY=UTXO_SPLITin.env.regtestand runpegout-cycle.sh full-cycle --repeat 3+check-btc-releaseScreenshots (if applicable)
Add screenshots or GIFs to help explain your changes.
Additional Notes
AI Code Review: Configurable Rebalance Strategy for Bridge Pegout
Commit:
5cea8c2— "Add configurable rebalance strategy for bridge pegout"Branch:
feature/FLY-2235Reviewer: Claude Opus 4.6 (AI-assisted review)
Date: 2026-03-13
1. Summary
This feature introduces a configurable rebalance strategy for the bridge pegout flow. Previously, the LP always sent the entire accumulated refund total to the RSK Bridge in a single transaction (
ALL_AT_ONCE). The newUTXO_SPLITstrategy splits the bridge conversion into multiple transactions ofBridgeTransactionMinsize each, producing multiple UTXOs on the BTC side.Motivation: When the Bridge releases BTC back to the LP, each bridge transaction produces one UTXO. With
UTXO_SPLIT, the LP receives N smaller UTXOs instead of one large one, improving UTXO management for future pegout operations.2. Files Changed
internal/usecases/pegout/bridge_pegout.goRebalanceStrategytype,ParseRebalanceStrategy(),runAllAtOnce(),runUtxoSplit()internal/usecases/pegout/bridge_pegout_test.gointernal/configuration/environment/environment.goRebalanceStrategyfield toPegoutEnvinternal/configuration/environment/environment_reader_test.gointernal/configuration/registry/usecase.goBridgePegoutUseCaseinternal/configuration/registry/usecase_test.gointernal/configuration/registry/watcher_test.gointernal/adapters/entrypoints/watcher/pegout_bridge_watcher_test.gocmd/application/lps/application.goNewUseCaseRegistryreturning(*UseCaseRegistry, error)docker-compose/local/docker-compose.lps.ymlREBALANCE_STRATEGYto container envsample-config.envREBALANCE_STRATEGY=ALL_AT_ONCE3. Architecture & Data Flow
Trigger Path
PegoutBridgeWatcherticks every 5 minutesRefundPegOutSucceededstateBridgePegoutUseCase.Run(ctx, quotes...)Run()calculatestotalValue = Σ(Value + CallFee + GasFee)for all quotestotalValue < BridgeTransactionMin→ skip (normal, logged as info)rskWalletMutexrunAllAtOnce()orrunUtxoSplit()based onstrategy4. Splitting Algorithm (UTXO_SPLIT)
Why this is rounding-safe: All arithmetic uses Go's
math/big.Int(arbitrary precision integers). The remainder is computed by subtraction (total - N×min) rather than modulo, sofirstChunk + (N-1)×min = totalis an algebraic identity, not an approximation.Gas Calculation
For
ALL_AT_ONCE, gas is1 × gasPerTx. ForUTXO_SPLITwith N transactions, gas scales linearly asN × gasPerTx.5. Code Review Findings
5.1 Correctness
big.Int.Divtruncates toward zeroremainder = total - N×min(subtraction, not modulo)TestUtxoSplit_AmountIntegritymin + remainder ≥ min; rest =minconfig := blockchain.NewTransactionConfig(totalValue, ...)requiredBalance = totalValue + N×gasPerTxbridgeMin.Copy()prevents mutation.Copy()before passing to tx configALL_AT_ONCEis the default (line 94)5.2 Error Handling
TxBelowMinimumError, logged as info by watcherInsufficientAmountErrorreturnedBridgeTxFailed, error returnedUpdateRetainedQuotesfails5.3 Design Observations for Peer Discussion
A. Partial Success Handling in UTXO_SPLIT
When N≥2 and tx K fails (K>1), transactions 1..K-1 have already been sent to the bridge and cannot be rolled back. The code marks all quotes as
BridgeTxFailed:What happens:
BridgeTxFailed, so the watcher won't re-process themAssessment: This is a conservative design choice — it prevents double-sending by marking all quotes as terminal. The LP receives the partial BTC but the accounting is pessimistic. An operator would need to reconcile manually.
Alternative (not necessarily better): Track per-tx success and mark quotes proportionally. But this adds complexity and partial state management that may not be worth it given the rarity of mid-split failures.
B.
updateQuotesUses Last Receipt OnlyIn the split loop,
updateQuotesis called once with the final receipt:On success (all N txs complete),
receiptis from the last tx. This meansBridgeRefundTxHashon all quotes points to tx N, not tx 1. ForALL_AT_ONCEthis is fine (one tx). ForUTXO_SPLIT, it means quote records reference only the last bridge tx hash.Impact: The other tx hashes are logged (debug level) but not persisted in the database. If an operator needs to audit all bridge transactions for a rebalance cycle, they'd need to check logs or chain state.
C.
numTxsReturned as*WeiFrom DivisionThe error from
Divis discarded. This is safe becausebridgeMinis validated upstream (it passes theBridgeTransactionMin.Cmp(totalValue) > 0check at line 79, confirmingbridgeMin > 0), so division by zero cannot occur. However, the discarded error is a minor code smell.D. Default Strategy is ALL_AT_ONCE
The
switchstatement defaults torunAllAtOnce:This is safe: even if a new strategy value were somehow injected, the system falls back to the original single-tx behavior. The double validation (env + parsing) prevents this in practice.
6. Test Coverage Assessment
Existing Tests (from the cherry-picked commit)
testBridgePegoutUseCaseSuccesstestBridgePegoutUseCaseValueBelowMinimumtestBridgePegoutUseCaseQuotesNotRefundedtestBridgePegoutUseCaseWalletBalanceErrortestBridgePegoutUseCaseWalletWithoutBalancetestBridgePegoutUseCaseTxFailstestBridgePegoutUseCaseUpdateFailstestUtxoSplitSuccesstestUtxoSplitNoSplitWhenN1testUtxoSplitBelowMinimumtestUtxoSplitExactMultipletestUtxoSplitFailMidSplittestUtxoSplitInsufficientGasTestParseRebalanceStrategyAdded Tests (on this branch)
TestUtxoSplit_AmountIntegrity(8 sub-tests)Cases in the integrity test:
Coverage Gaps (Minor)
bridgePegoutTestWatchedQuotes. A test with many quotes where Value+CallFee+GasFee produces an interesting total would add confidence.testUtxoSplitFailMidSplittests tx2 failure. A test where tx1 itself fails would verify the early-exit path at lines 170-175.7. Security Assessment
7.1 LPS Flow Security
contracts.Bridge.GetAddress()— hardcoded from contract registry, not user-controlledrskWalletMutexserializes all bridge sends; quotes transition to terminal state (BridgeTxSucceeded/BridgeTxFailed) atomicallyREBALANCE_STRATEGYvalidated at startup — invalid value prevents boot. No runtime mutationbig.Intarithmetic — no overflow possibleN × gasPerTxbefore any send. If insufficient, rejects earlyALL_AT_ONCE. No path to inject arbitrary strategy7.2 Rootstock Federation Bridge Flow Security
This section examines how the UTXO_SPLIT strategy affects the RSK Bridge and federation nodes.
Bridge Perspective
The RSK Bridge at
0x0000000000000000000000000000000001000006processes incoming RBTC transfers as pegout requests. Each transfer that meets the minimum lock value is queued for BTC release.bridgeMin(orbridgeMin + remainderfor the first). SincebridgeMinis configured to matchBridgeTransactionMin(the Bridge's own minimum), all txs meet the Bridge's acceptance threshold.getNextPegoutCreationBlockNumber). More queue entries mean more BTC outputs in the release tx, increasing its size. For N≤10 this is negligible; for very large N (e.g., 66 in the 100 RBTC test case), the Bridge release tx could be significant.BTC Side
bridgeMinBTC each, instead of 1 large UTXO. Improves the LP's ability to serve future pegout requests without change-output management.getFeePerKb). For typical N (2-5), this is minimal. The LP does not pay this fee — it's deducted from the Bridge's pool.bridgeMin(1.5 RBTC = 1.5 BTC in regtest), far above Bitcoin's dust threshold (~546 satoshi).Timing & Ordering
rskWalletMutexensures serialization. EachSendRbtccall should increment the nonce atomically. If the wallet implementation handles nonces correctly, this is safe.rskWalletMutexprevents concurrent bridge sends. A split of 66 txs at the gas price/limit used would complete well within 5 minutes. No race condition.7.3 Attack Scenarios Considered
7.4 Security Verdict
No security vulnerabilities identified. The feature is a clean extension of the existing rebalance flow. The
UTXO_SPLITstrategy interacts with the Bridge in the same way asALL_AT_ONCE— it simply sends multiple qualifying transfers instead of one. The Bridge's native handling of pegout queues, fee calculation, and federation signing is unaffected.The one operational risk (partial failure accounting mismatch) is a design trade-off, not a vulnerability. It favors safety (no double-send) over convenience (manual reconciliation).
8. Compatibility with This Branch
The main feature commit was tested on
masterand then moved ontofeature/FLY-2235. Key differences on this branch:LBC_ADDRreplaced by split contract addressescontracts.Bridge, not LBCFeeCollectorAddressremovedProviderTypechanged from field to methodAllowedOriginsCORS field addedNewUseCaseRegistryreturn type change(*UseCaseRegistry, error)All unit tests pass on the branch (32 packages). Build compiles cleanly. No merge conflicts.